fix(eval): close raw tunnels at the layer that owns the sockets - #3017

The follow-up is aligned with the parent PR review: removing the unused tcp_start/tcp_message flow.kill hooks, keeping a single pre-relay next_layer path, using isinstance, and asserting both CloseConnection commands are all good changes.

One issue remains before merge: constructing CloseRawLayer through Layer.init appends it to context.layers without removing the TCPLayer being replaced, leaving a stale protocol layer in the stack. The test uses a fake that does not register itself in context.layers, so it misses this behavior.

Please preserve the layer-stack invariant and extend the test accordingly. After that correction, this is ready to merge.

A fresh live probe changes the conclusion from the earlier review: removing tcp_start and tcp_message is not safe with the current replacement.

In mitmproxy 12.2.3, script addons receive next_layer before the built-in classifier has populated nextlayer.layer. A live probe showed nextlayer.layer was None at both script hook calls, so the isinstance(current, TCPLayer) branch never ran. After this PR removed the TCP hooks, a CONNECT raw tunnel relayed bytes in both directions and produced no raw_tunnel audit record.

HTTP 101 upgrades are a separate path. Mitmproxy constructs their TCPLayer inside the HTTP layer without another next_layer hook. The same probe confirmed bidirectional raw payload and no audit record for that path as well.

This also changes the earlier layer-stack finding. The stale TCPLayer is real, but it is secondary: the closer is not reached on either production path. Fixing the stack alone would not make this safe.

The current test hides the problem by constructing a fake with .layer already set. In production it is still None when the script hook runs. CI also runs this test without mitmproxy installed, so the fallback imports and fake types do not verify the real addon order or connection behavior.

I suggest keeping the existing TCP hooks until an equivalent replacement has been exercised through a real proxy. A complete replacement needs to cover both raw-protocol selection and HTTP 101 upgrades. One clean option is to let mitmproxy's native rawtcp=false setting own the fail-closed guarantee, then keep narrow hooks for audit. If the custom closer remains, it should run only after the built-in classifier has selected TCPLayer, remove that layer from context.layers, and handle the 101 path separately.

The verification should use mitmproxy 12.2.3 and assert zero relayed bytes, the expected audit record, connection closure, normal HTTPS behavior, and WebSocket compatibility.

I used Claude for the live mitmproxy probe and Codex reviewers for independent source and lifecycle checks, then verified the current head and CI state.

Thanks for following up with real mitmproxy coverage and for restoring the TCP hooks after the production-order evidence. The current head does prevent payload relay for raw CONNECT and HTTP 101 paths without breaking ordinary HTTP(S) or WebSocket traffic.

I found two remaining P2 issues before approval; the inline comments contain the reproductions and requested verification.

From first principles, the remaining production problem is ownership of connection closure. The current next_layer -> CloseRawLayer branch is not reached in the observed addon order because the script hook sees layer=None, while the reachable tcp_start / tcp_message path uses TCPFlow.kill(), which marks the flow but does not close either transport. A long-lived peer can therefore retain both sockets even though payload is suppressed. Please make one production-reachable layer/connection seam the sole closing authority, then remove the superseded parallel path and its fake-only tests.

The live harness belongs in this PR because unit fakes cannot prove the mitmproxy lifecycle, but it should assert proxy-initiated EOF/reset against a long-lived origin and isolate each run's Docker resources. Once the final path is settled, please also update the PR title and body: they still describe the earlier next_layer-only design and removal of the TCP hooks.

I verified 14/14 focused Python unit tests and 5/5 live tests on the pinned image, then reproduced the open-socket behavior with long-lived upstreams. CI is green, although the opt-in live suite is skipped by default. I found no P0 or P1 issue.

Disclosure: I used three Codex reviewers and Claude for independent lifecycle, test-authenticity, and simplification checks. I reproduced the surviving findings and own this review; AI assistance does not replace the repository's required independent human review.

Actionable comments posted: 2

Pull request overview

This PR adjusts the eval egress proxy’s handling of “raw tunnel” situations so that raw CONNECT and non-WebSocket HTTP 101 upgrades are closed via CloseConnection at the layer that owns the sockets, rather than relying on TCPFlow.kill() after bytes may already have been relayed.

Changes:

• Add a CloseRawLayer and next_layer pre-classification logic to close raw TCP early and ensure both client/server transports are closed.
• Force rawtcp=false (entrypoint + addon configure) and audit non-WebSocket HTTP 101 upgrades as raw_tunnel.
• Add unit tests for the new next-layer behavior and an opt-in live Docker harness to validate behavior against the pinned mitmproxy image.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

`MAKA_EVAL_EGRESS_PROXY_TEST=1 python3 harbor/test_egress_filter_live.py` starts the
pinned mitmproxy image and asserts raw CONNECT and HTTP 101 upgrades relay no
bytes, write `raw_tunnel`, and leave HTTPS and WebSocket working; it needs a Docker
daemon, the pinned image, `python:3.12-slim`, and outbound network, and skips otherwise. This URL policy is a blocklist for known

        curl = subprocess.run(

Actionable comments posted: 2

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

        if not docker_image_present(ORIGIN_IMAGE):
            raise unittest.SkipTest(f"{ORIGIN_IMAGE} is not present")

`MAKA_EVAL_EGRESS_PROXY_TEST=1 python3 harbor/test_egress_filter_live.py` starts the
pinned mitmproxy image and asserts raw CONNECT and HTTP 101 upgrades relay no
bytes, write `raw_tunnel`, and leave HTTPS and WebSocket working; it needs a Docker
daemon, the pinned image, `python:3.12-slim`, and outbound network, and skips otherwise. This URL policy is a blocklist for known
benchmark and public-solution contamination surfaces, not a complete defense against a deliberately

Thanks for moving raw-tunnel rejection to the layer that actually owns the sockets. I reviewed this head with two independent @reviewer passes plus a read-only ollama-cloud/deepseek-v4-flash:high pass, and checked the disputed path against mitmproxy 12.2.3's classifier/event ordering.

The problem is correctly defined and CloseRawLayer is the right authority. The remaining correctness gap is that the pre-classification decision is binary even when the protocol prefix is still incomplete: a one- or two-byte TLS ClientHello can be irreversibly classified as raw before the third byte arrives. Current CI is green, but the existing tests only cover a complete TLS prefix and cannot exercise this TCP fragmentation.

The smallest first-principles fix is a tri-state classifier at this seam: confirmed HTTP/TLS, confirmed raw, or undecided. Keep buffering while a prefix can still become a TLS record or HTTP request, and install CloseRawLayer only for confirmed raw traffic. Once that is the sole production authority, remove the unreachable post-classifier replacement branch and its fake-only test.

No local repository test suite was run by Codex; one reviewer performed focused source-level verification against the pinned mitmproxy implementation. Codex coordinated the passes and performed the final adjudication; external-model output was treated as unverified until checked against the code.

Thanks @1625567290 — reviewed at exact head 83a4283b24470c214367fa956b4dd4c74ec8ef36. One P2 and one P3; no P0/P1.

The ownership change itself reads correctly: assigning CloseRawLayer while the layer is still None puts the close on the path the production hook actually takes, and the resulting CloseConnection for both client and server is handled by mitmproxy's command loop rather than by killing the flow and leaving the socket open. Double-close is safe on that side — once a connection's transport has been popped, the command is a no-op, and close_connection cancels an already-cancelled handler idempotently. So the peer-disconnected-first and both-paths-close-at-once cases do not leak or throw.

The unit tests are good where they apply: fragmented TLS prefixes down to 1–2 bytes, HTTP prefixes arriving in pieces, SSH banners, binary payloads, and an explicit assertion that CloseRawLayer emits both CloseConnection commands.

The P2 is that none of that reaches CI — details inline.

P3, redundant configuration. entrypoint.sh already passes --set rawtcp=false on the mitmdump command line, and the configure() hook sets ctx.options.rawtcp = False again. It is guarded and has no side effect, so this is only a note: two places now express the same setting, and a future change to either one can silently disagree with the other.

The kill_flow fallback on tcp_start / tcp_message and its behaviour were already raised in earlier reviews on this PR; not repeating that here.

This review was AI-assisted. It is not a substitute for independent human review by a committer.

Incremental review at exact head db3c117b25079e27fccbc539eee5c28f4ecdb1fa, focused on what changed since the last reviewed head.

The previously-raised P2 is genuinely closed at the wiring level. ci.yml:188-198 now pulls and builds the image when Eval is selected and forces the live env flag, and a failure there fails the job — the live suite is no longer excluded from CI by construction. Two new findings, though, and the first one affects ordinary traffic.

[P1] A normally-fragmented TLS ClientHello is killed as raw TCP — packages/eval/harbor/egress_filter.py:133-134, 259-265

def starts_like_tls_record(data: bytes) -> bool:
    return len(data) >= 3 and data[0] == 0x16 and data[1] == 0x03

looks_like_raw_tcp returns early with False only when this is true. With one byte (0x16) or two (0x16 0x03) received so far, the length test fails, so the TLS escape hatch doesn't fire. Control flow then reaches _still_could_be_http(data_client), which is False for a control byte — no valid request line begins with 0x16. looks_like_raw_tcp therefore returns True, and next_layer:195-200 immediately installs CloseRawLayer.

The connection is killed before the third byte arrives, so the bytes that would have identified it as TLS never get a chance to. This is an ordinary path — TCP is free to deliver a ClientHello in small segments, and no malformed or hostile input is required.

The docstring above the function shows the author already reasoned about exactly this hazard for HTTP: copying mitmproxy's probably_no_http would misclassify GET / GET while the request line is still arriving, so the code deliberately keeps waiting. TLS needs the same treatment and currently doesn't get it.

Suggested shape: make the classifier tri-state rather than boolean — raw / not-raw / undecided — and return undecided while the buffer is still a viable TLS prefix (data == b"\x16", or data == b"\x16\x03"). That matches how the HTTP side already behaves: don't decide until the evidence can exist.

Verified against the pinned 12.2.3 image: the detector returns False for 16, False for 1603, and True only at 160301.

[P2] The live harness doesn't prove the origin socket is closed — packages/eval/harbor/test_egress_filter_live.py:294-305, 411-430

The test waits for the client side to see EOF/reset and asserts the origin received zero payload bytes plus the expected audit record. Neither establishes that the origin-side socket was closed: the origin stats carry no peer-EOF or close counter, so a leaked origin connection that simply never receives data would pass this test unchanged.

The unit test's two CloseConnection commands aren't a substitute — they prove the addon emitted both, not that the real mitmproxy loop closed both ends. Recording and waiting on EOF at the origin would close the gap, along with a case that is explicitly a non-WebSocket upgrade so the tunnel path is exercised deliberately rather than incidentally.

CI status

Exact-head audit is green, but test is completed/cancelled — the run (32623743066) was cancelled during Install dependencies, so the live step was skipped. There is no hosted exact-head evidence that the newly-wired live suite passes. Worth a re-run before this is considered gated.

Validation scope. Local runs at this head: unit 18/18, planner 29/29, Docker live 5/5. Those five live cases don't cover either finding above — neither a fragmented ClientHello nor an origin-side close assertion is among them, which is why both survived a green local run.

Thanks for taking both items head-on. The origin-close proof is genuinely closed now, and I want to be specific about why, because it is the part that was hardest to get right. The remaining item still blocks.

The origin-close finding is closed

packages/eval/harbor/test_egress_filter_live.py:51-126 increments its counters inside the origin's own finally block when the raw-socket and 101-handler receive loops exit, and :445-457,491-516 read those counters from the origin container independently, wait for them to advance, and only then assert *_recv == 0 with no banner and an audit hit. That is an independent observation of the origin peer, not client EOF, handler cancellation, server close, or a zero-length payload standing in as self-proof. Verified by running the raw CONNECT and non-WebSocket 101 cases directly: 2/2 green in 3.096s.

[P1] Fragmented 1- and 2-byte TLS record prefixes still hang on the real mitmproxy path

egress_filter.py:268-273 now uses _could_start_tls_record to withhold CloseRawLayer for a leading 16 or 16 03, so the connection is no longer torn down early. But nothing re-drives classification once the rest of the ClientHello arrives: the real mitmproxy 12.2.3 classifier does not re-decide or complete the handshake, so the connection moves from "closed too early" to "never resolved".

This PR's own live tests say so. test_egress_filter_live.py:350-399,478-483 covers the 1-byte and 2-byte subcases, and both time out in sock.recv with TLS still at SSLWantReadError. Reproduced independently against the pinned Docker image at this exact head — both subcases time out after 23.194s.

The hosted result agrees: CI run 32639766029 attempt 2 at 0f4de78a2110d0bcddb080efc545082c84a6281b is a terminal failure, and the test check is completed/failure. In that live step, 4 of 6 tests pass and the two failures are exactly these fragmented subcases; every preceding lint, format, build, typecheck and affected-test stage is green, and audit is completed/success. So this is not flaky infrastructure — the required check is red on precisely the behaviour under discussion.

One caveat on the unit-level evidence: the addon and fallback detector tests pass, but they call next_layer a second time by hand. That proves the helper's tri-state logic in isolation; it cannot show that the production layer stack ever performs that second call. The two real-event-loop tests are the ones that speak to production, and they fail.

What the fix needs. The undecided state has to be handed to a layer that will genuinely re-classify and make progress as later bytes arrive, and both real paths must go green. Please also confirm the behaviour holds whether or not mitmproxy.net.tls.starts_like_tls_record is importable — the original finding was anchored on the local 3-byte fallback, so a fix proven only with the upstream helper present would not close it.

ran: the pinned-Docker live suite at this exact head (raw CONNECT and non-WebSocket 101 green in 3.096s; the two fragmented subcases timing out after 23.194s), plus independent verification of the hosted check state for this commit.
did-not-run: anything outside packages/eval/harbor; the nine pre-existing commits were not re-reviewed, since git range-diff shows them patch-equivalent to the previously reviewed revision.

Reviewed exact head 6fab34c0257a1f91364018f74813bf61e1c6c762: NO-GO, with one P1 and one P2 inline.

The long-running ownership dispute is mostly resolved correctly. On the shipped raw CONNECT path, the pre-classification CloseRawLayer is the component that owns both transports and emits both CloseConnection commands. On non-WebSocket HTTP 101, rawtcp=false leaves closure to mitmproxy's HTTP layer. I independently exercised both paths on mitmproxy 12.2.3: the client received no origin banner, the origin independently observed peer EOF, and the raw_tunnel audit was present. The earlier TCPFlow.kill()-as-authority and origin-close-proof findings are therefore closed.

The remaining production blocker is specifically the fragmented TLS transition: returning without assigning a layer is not an undecided state once the built-in addon runs later in the same hook dispatch. The current live test also cannot become green after that is fixed because its client does not trust the per-run proxy CA. Details and reproductions are inline.

Validation at this exact head: fallback and real-mitmproxy unit suites 18/18; CI planner 30/30; git diff --check clean; direct real-mitmproxy probes for raw CONNECT, raw 101, and TLS first fragments. Fresh machine state is OPEN / MERGEABLE / BLOCKED. audit is COMPLETED/SUCCESS; required test is currently IN_PROGRESS on run 32644324917, so there is no exact-head gate-complete basis for APPROVE in addition to the findings above. This is COMMENT-only; no merge action.

Approving. The ownership argument holds: on main, next_layer returns immediately unless the layer is already a TCPLayer, and the script hook fires before the built-in classifier assigns one — so on the production CONNECT path that branch never runs and the raw tunnel is never closed. Assigning CloseRawLayer ahead of the classifier, letting the built-in HTTP layer close 101 upgrades via rawtcp=false, and keeping the isinstance branch for already-classified paths puts each close where that socket is actually owned, with no second authority.

I checked the parts most likely to hide a leak and found none: _next_layer_bytes returning b"" on error falls through to the built-in classifier rather than skipping the close, CloseRawLayer yielding CloseConnection twice is harmless, kill_flow swallowing exceptions only ever costs an extra attempt, and apply_http_policy fails closed with a 503.

The +995/-10 is not new machinery. Production logic is egress_filter.py +139/-5; the bulk is a real Docker/mitmproxy live suite (606 lines) plus unit tests (+215) and the CI wiring to run them.

One behaviour worth stating explicitly, because I went looking for a hole here and it is worth recording that there isn't one. This head commits to TLS on a 1–2 byte fragment prefix and assigns the layers immediately, giving up the chance to reclassify with more data. I tested whether a raw tunnel could exploit that: against the pinned image, four CONNECT payloads — plain, \x16, \x16\x03, \x16\x03\x01, each followed by garbage — were all closed, with zero bytes relayed to the origin and one raw_tunnel audit record each. Pinning the TLS layer makes the fake client fail the handshake, and the teardown still passes through the audited path.

One non-blocking gap is inline.